Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adjust theme types to match internal and regular usage #3279

Merged
merged 16 commits into from
Oct 18, 2022

Conversation

Drakeoon
Copy link
Contributor

@Drakeoon Drakeoon commented Jul 26, 2022

During the development we came to the conclusion that useTheme right now has to serve too many roles - support two themes at the same time for internal components and later on support just one theme when used by developers including react-native-paper into their applications

Summary

  • Added useInternalTheme hook for internal usage
  • Added withInternalTheme HOC for internal usage
  • Renamed global type definition
  • Change usage inside internal components
  • Raised a PR in @callstack/react-theme-provider to allow typing hooks
  • TypeScript support for custom themes for withTheme HOC is still in progress, probably not a part of this PR

Test plan

Due to the usage of global index.d.ts files I had troubles with testing the implementation inside example folder. My idea now is to create a separate app, include react-native-paper from this branch and see if global theme overrides work properly

@Drakeoon
Copy link
Contributor Author

@lukewalczak CircleCI tests fail due to missing implementation in @callstack/react-theme-provider - I'll raise the PR today

@callstack-bot
Copy link

callstack-bot commented Sep 5, 2022

Hey @Drakeoon, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@lukewalczak lukewalczak force-pushed the feat/adjust-overriding-theme-types branch from 7076433 to ecf20ac Compare September 5, 2022 11:11
@github-actions
Copy link

github-actions bot commented Sep 5, 2022

The mobile version of example app from this branch is ready! You can see it here

.

@lukewalczak lukewalczak marked this pull request as ready for review September 5, 2022 15:47
@github-actions
Copy link

github-actions bot commented Oct 5, 2022

The mobile version of example app from this branch is ready! You can see it here

.

Copy link
Member

@lukewalczak lukewalczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small grammar suggestion and adding useTheme in example

docs/pages/2.theming.md Outdated Show resolved Hide resolved
docs/pages/2.theming.md Outdated Show resolved Hide resolved
docs/pages/2.theming.md Outdated Show resolved Hide resolved
docs/pages/2.theming.md Outdated Show resolved Hide resolved
docs/pages/2.theming.md Outdated Show resolved Hide resolved
docs/pages/2.theming.md Outdated Show resolved Hide resolved
docs/pages/2.theming.md Outdated Show resolved Hide resolved
docs/pages/2.theming.md Outdated Show resolved Hide resolved
docs/pages/2.theming.md Outdated Show resolved Hide resolved
@github-actions
Copy link

The mobile version of example app from this branch is ready! You can see it here

.

1 similar comment
@github-actions
Copy link

The mobile version of example app from this branch is ready! You can see it here

.

@lukewalczak lukewalczak force-pushed the feat/adjust-overriding-theme-types branch from d205270 to 871a875 Compare October 17, 2022 10:40
@github-actions
Copy link

The mobile version of example app from this branch is ready! You can see it here

.

@github-actions
Copy link

The mobile version of example app from this branch is ready! You can see it here

.

Copy link
Member

@lukewalczak lukewalczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @Drakeoon 🔥!

@lukewalczak lukewalczak merged commit 71ad62a into main Oct 18, 2022
@lukewalczak lukewalczak deleted the feat/adjust-overriding-theme-types branch October 18, 2022 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants